Skip to content

Conversation

stemangiola
Copy link
Contributor

No description provided.

@stemangiola stemangiola requested a review from Copilot June 28, 2025 23:36
@stemangiola stemangiola linked an issue Jun 28, 2025 that may be closed by this pull request
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a covariate layer in the header formatting, adds tests and example scripts, updates documentation, and extends utility functions for custom formatting.

  • Adds a format_covariate_header function and integrates it into the tbl_format_header.SE_print_abstraction method.
  • Introduces new unit tests under tests/testthat/ and a standalone debug script.
  • Updates the README examples and session info, and extends pillar utilities and NAMESPACE imports.

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
vignettes/Introduction.R Deleted outdated introduction vignette
tests/testthat/test-header-formatting.R Added snapshot tests for format_covariate_header
test_header_formatting.R Added standalone debug/example script (should be reviewed)
README.md Updated example output rows and sessionInfo details
R/tidyprint_1_utlis.R Added format_covariate_header and extended formatting logic
R/print_methods.R Adjusted separator row computation and attribute packing
R/pillar_utlis.R Extended pillar___format_comment with strip.spaces
NAMESPACE Added new importFrom entries for dplyr, purrr, etc.
Comments suppressed due to low confidence (3)

R/pillar_utlis.R:3

  • This function now calls get_extent, but get_extent isn't imported in NAMESPACE; add importFrom(pillar, get_extent) so the function resolves correctly at runtime.
pillar___format_comment <- function (x, width, strip.spaces = TRUE)

NAMESPACE:19

  • Missing imports for newly used functions pillar::style_subtle and pillar::get_extent; please add importFrom(pillar, style_subtle, get_extent) to NAMESPACE.
importFrom(cli,col_br_black)

@@ -0,0 +1,139 @@
# Test script for header formatting function
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This standalone debug/example script appears to be for local testing; consider removing it or moving it under tests/ so it does not clutter the root directory.

Copilot uses AI. Check for mistakes.


covariate_indices <- which(printed_colnames %in% covariate_names)
covariate_widths <- separator_row[printed_colnames[covariate_indices]] |> purrr::map_int(nchar)
total_covariate_width <- sum(covariate_widths) + length(covariate_widths) + 3 # To compensate the white spaces of the tibble
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using unexplained magic numbers like + 3; consider computing the extra spacing based on actual separator characters or document the reason for this offset.

Suggested change
total_covariate_width <- sum(covariate_widths) + length(covariate_widths) + 3 # To compensate the white spaces of the tibble
# Compute extra spacing dynamically based on separator characters
separator_spacing <- nchar("|") * 2 + 1 # Account for '|' at both ends and one space
total_covariate_width <- sum(covariate_widths) + length(covariate_widths) + separator_spacing

Copilot uses AI. Check for mistakes.

header_row[covariate_indices[-1]] <- ""

# Add row ID spacing at the beginning
header_row <- c(paste(rep(" ", number_of_total_rows |> nchar() - 3), collapse = ""), header_row)
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calculating row-ID spacing using nchar(number_of_total_rows) - 3 is brittle; consider deriving the padding from the actual row ID width rather than hard-coding - 3.

Suggested change
header_row <- c(paste(rep(" ", number_of_total_rows |> nchar() - 3), collapse = ""), header_row)
row_id_width <- nchar(total_rows)
header_row <- c(paste(rep(" ", row_id_width), collapse = ""), header_row)

Copilot uses AI. Check for mistakes.

number_of_features,
number_of_samples,
assay_names %>% paste(collapse=", ")
)), after = 1)
)))
# , after = 1)
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove leftover commented-out code snippets to keep the implementation clean and maintainable.

Suggested change
# , after = 1)

Copilot uses AI. Check for mistakes.

stemangiola and others added 4 commits June 29, 2025 10:00
@zhanchen zhanchen merged commit 978e3a6 into main Aug 29, 2025
0 of 3 checks passed
@stemangiola
Copy link
Contributor Author

@zhanchen this PR I dont think works for arbitrary datasets. I tagged into an issue about the non existance of a variable that counts the table size in its renfered form.

This was not meant to be merged I think. Unless you solved that issue and tested extensively on many datasets.

@zhanchen
Copy link
Member

zhanchen commented Aug 30, 2025

@stemangiola
Yes, I fixed the covariate-layer print issue and committed to the branch before the merge.
Now the covariate-layer can align with the covariate column under varying widths and shall be removed when the width is too small and no covariate column was actually printed.
screenshot_20250830101425
screenshot_20250830101409
screenshot_20250830101343
I will test it with datasets in the next couple days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explicit covariate compartment
2 participants